Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run tests with pytest #963

Merged
merged 48 commits into from
Dec 11, 2018
Merged

Run tests with pytest #963

merged 48 commits into from
Dec 11, 2018

Conversation

craigds
Copy link
Contributor

@craigds craigds commented Oct 2, 2018

What does this PR do?

https://trac.osgeo.org/gdal/wiki/rfc72_pytest

The intent is to use pytest to run the GDAL test suite.

Large parts of this PR are automated, but manual pieces are in their own separate commits for reviewability. The automated commits are probably too large to review on github.

Notable changes and their implications

  • tests are now run with cd autotest ; pytest. (The first time you may need to pip install -r requirements.txt to install pytest)
  • All tests now run in a single process (they were previously forked for each test module). In future it's possible to multi-process them and parallelise, but I avoided doing that in this PR for simplicity. This means that:
    • errors during test collection are now loud, and immediately fail the entire test run with a traceback. Previously things like syntax errors in files and errors at module level were easy to miss.
    • A single segfault will kill the entire test run dead. (note: This might be handle-able via the xdist plugin, it seems to restart failed worker processes; is that behaviour desirable though? Segfaults are bad ;) )
  • It's now possible to run individual tests, instead of just entire files. However, tests are not yet independent of each other. So that might cause the tests to behave differently than if you ran the whole module. YMMV.
  • All tests have been renamed test_<name>. Pytest finds tests by matching names against a regex (this is the default regex).
  • test_py_scripts.run_py_script was modified to always run the script as a subprocess. The stdout capturing of the original method did strange things with pytest. This change broke some tests that relied on passing files in the /vsimem/ root to scripts, so I've changed those to use the tmp/ root instead.

Changes not included in this PR

  • I haven't made test functions independent of each other. You still have to run an entire module at a time for most modules. I intend to do this in future but it's out of scope for this PR.
  • I haven't removed all the global gdaltest.<drivername>_drv variables and replaced them with pytest fixtures. That's probably related to making functions independent, and is also future work.
  • I haven't done a style cleanup. In particular my automated changes might make code style worse in some respects, and I haven't fretted too much about that. For example, I changed if statements to asserts. Some asserts have turned into really long lines as a result. I think automated style cleanup could easily be done as a separate PR after all this, probably using black.

What are related issues/pull requests?

Tasklist

  • Get test collection working

  • Manually parametrize the 50-ish remaining dynamically-generated tests (e.g. gdaltest_list.append((ut.testProj, item[6])))

  • Automated conversion

    • rename all tests to test_*
    • compatibility shim for old tests - Instead, I'm aiming to replace all return 'skip' / 'fail'. It's nearly there
    • generate assertions from post_reason/return 'fail' calls where possible
    • replace all skip/fail/success return values
    • remove extra ../pymod entries from sys.path, now that they're all running in one process
    • remove __main__ block and gdaltest_list from test files
  • Manually review remaining gdaltest_list.append(...) cases and add pytest.skip() as appropriate. Possibly automate some of this somehow?

  • Clean up remaining fails manually. Remaining failures checklist

  • Ensure the slow/internet tests are still marked as such and skipped by default. (note: check autotest: update environment variable requirements #1054)

  • Add documentation:

    • a straightforward install process for pytest itself
    • how to run tests; caveats
  • Consider adding extra niceties to make sure people still like me after this huge PR

    • Find a way to stop writing tmp files into the source tree (difficult due to each test already having an overridden CWD. Try to remove that maybe?) (note: considered, but rejected; this PR is big enough and there's no easy one-line change that can do this. Future work)
    • Use pytest-sugar to make test output pretty. Make sure it works with travis output buffering or disable it in CI. (note: disabled in CI, also added --color=no to avoid ansi control chars in travis' raw logs)
    • Maybe use xdist plugin to parallelize tests across multiple processes, to speed up runs. (note: considered, but there's enough going on without introducing the indeterminacy of parallel tests. Future work)
  • Review

  • Adjust for comments

  • All CI builds and checks have passed

@craigds craigds force-pushed the pytestify branch 2 times, most recently from 92dd2fb to 2537360 Compare October 4, 2018 10:32
@craigds
Copy link
Contributor Author

craigds commented Oct 4, 2018

⚠️ I'm going to be rebasing & force-pushing this repeatedly until it is merged. The difficulty of merging upstream changes into this is prohibitive; it's much easier to just rebase and re-run the automated changes from scratch.

So, basically, please clone this and have a look at it, just be prepared to force-reset your local copy when I update it 😄

@craigds craigds force-pushed the pytestify branch 7 times, most recently from 4a4a190 to 4d7963e Compare October 11, 2018 22:12
@craigds craigds force-pushed the pytestify branch 14 times, most recently from cae613f to 1c9b78e Compare October 19, 2018 11:17
@craigds craigds force-pushed the pytestify branch 6 times, most recently from 50d0e31 to bc96bde Compare October 24, 2018 08:33
@craigds
Copy link
Contributor Author

craigds commented Dec 10, 2018

🎉

rebased it to squash some of the obviously-related commits together, but I'm mostly happy with the history and don't want to spend ages re-ordering it.

@rouault merge when you're happy 😄

@craigds
Copy link
Contributor Author

craigds commented Dec 10, 2018

... unless you think the commit history needs improving in which case I can spend a little longer on it of course.

@rouault
Copy link
Member

rouault commented Dec 10, 2018

unless you think the commit history needs improving

No, that'll be fine like that. This will reflect well the effort you put in this :-)

@craigds
Copy link
Contributor Author

craigds commented Dec 11, 2018

frustrating OSX CI error; no changes since previous successful build 🤦‍♂️

@craigds
Copy link
Contributor Author

craigds commented Dec 11, 2018

At @schwehr's suggestion I did a quick performance comparison on my vagrant box:

master:
589
690
693
(average 657s)

pytestify:
707
768
759
(average 744s)

So basically this adds 87s on to test runs.

I suppose some performance drop is inevitable but I'm a bit surprised it's as large as that.

@craigds
Copy link
Contributor Author

craigds commented Dec 11, 2018

In the meantime, the OSX CI error went away with a rebuild 👍 ; maybe that one's unrelated to this effort

@craigds
Copy link
Contributor Author

craigds commented Dec 11, 2018

I don't think the increased test time is a problem particularly. I'd prefer to just mitigate it by disabling a few of the slower tests by default:

(output from pytest --durations=10)

137.55s call     gdrivers/wms.py::test_wms_16
127.26s call     gcore/vsis3.py::test_vsis3_read_credentials_ec2_expiration
27.98s call     ogr/ogr_gft.py::test_ogr_gft_ogr2ogr_spatial
17.39s call     ogr/ogr_carto.py::test_ogr_carto_test_ogrsf
16.74s call     ogr/ogr_gft.py::test_ogr_gft_write
16.21s call     gdrivers/mrf.py::test_mrf_lerc_with_huffman
14.00s call     gdrivers/gdalhttp.py::test_http_2
12.88s call     ogr/ogr_gft.py::test_ogr_gft_ogr2ogr_non_spatial
12.30s call     gdrivers/wms.py::test_wms_11
12.19s call     gdrivers/mbtiles.py::test_mbtiles_3

I won't do that here because it just makes the comparison less fair, but happy to do it as a followup PR.

@rouault rouault merged commit 59d0840 into OSGeo:master Dec 11, 2018
@rouault
Copy link
Member

rouault commented Dec 11, 2018

@craigds I've rebased my gdalbarn branch on top of master with the pytest changes, and I now get a "The job exceeded the maximum log length, and has been terminated." on the trusty_clang config : https://travis-ci.org/rouault/gdal/jobs/466516798 . This is apparently really related to the log length, and not the time out
Any idea how to fix that ?

@rcoup
Copy link
Member

rcoup commented Dec 11, 2018

Yeah, I think maybe we take away the pytest -vv ... in script.sh and swap it for -ra, so it won't print anything at all for passed tests, and still have verbose details for failures + errors. But there's a confusing set of options there, maybe @craigds can confirm?

@rouault
Copy link
Member

rouault commented Dec 11, 2018

At the same time, if we don't print anything for too long, Travis will kill us because of this. I'm not clear why the pytest output doesn't please Travis. Perhaps each line is just slightly longer than before, but the number of lines should be similar.

@rouault
Copy link
Member

rouault commented Dec 11, 2018

Using -ra did the job.

@craigds
Copy link
Contributor Author

craigds commented Dec 11, 2018

@rouault One less drastic thing we could do is add -o console_output_style=classic to pytest invocations, which would supress the percentage progress indicator and all the whitespace padding to the right of the test names. I already did that in gcc52_stdcpp14_sanitize environment; can't remember why

@craigds
Copy link
Contributor Author

craigds commented Aug 3, 2019

I gave a talk about this effort yesterday at Pycon AU.

Description: https://2019.pycon-au.org/talks/how-i-migrated-a-huge-oss-project-to-use-pytest
Video: https://www.youtube.com/watch?v=SLbOWiz4KSs

@rouault
Copy link
Member

rouault commented Aug 4, 2019

Thanks for the link, and thanks again for the incredible job you did ! I enjoy so much being able to write tests in a more compact way.

@QuLogic QuLogic mentioned this pull request Feb 8, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants